Add jwks parsing (abandoned) #112

dtiukalov · 2020-12-02T13:00:43Z

add jwks implementation to simple access to x5c to verify jwt:

auto decoded_jwt = jwt::decode(token);
auto jwks = jwt::parse_jwks(jwks_json_str);
if (jwks.has_x5c_id()) {
   auto issuer = decoded_jwt.get_issuer();
   auto x5c = jwks.get_x5c();
   if (jwks.get_key_id() == decoded_jwt.get_key_id()) {
      if (!x5c.empty() && !issuer.empty()) {
         auto verifier = jwt::verify()
            .allow_algorithm(jwt::algorithm::rs256(jwt::helper::convert_base64_der_to_pem(x5c), "", "", ""))
            .with_issuer(issuer)
            .leeway(60U); 
         std::error_code err;
         verifier.verify(decoded_jwt, err);
         jwt::error::throw_if_error(err);
     }
}
   else {
      std::cerr << "Key ids are not matched" << std::endl;
   }
}

add jwks implementation to simple access to x5c to verify jwt

prince-chrismc · 2020-12-02T14:44:50Z

Wow! 🏆 super exciting to see this added, thank you so much for your contribution ❤️

This would be a start towards #94 which I think a lot of people would benefit from!

There's a few key details missing in order for this to be merged

Test code
- Something similar to this test
- Feel free to add a new file

No base64 (required by the helper you are using)

You'll need to add a overload like this

jwt-cpp/include/jwt-cpp/jwt.h

Lines 420 to 421 in ca110ad

    
           template<typename Decode> 
        
           std::string convert_base64_der_to_pem(const std::string& cert_base64_der_str, Decode decode, std::error_code& ec) {

Example
- I think the code you wrote above is excellent, just stick it in the examples!

I very happy to see this PR get merged, let me know if you need more help! 🚀

prince-chrismc · 2020-12-07T19:17:59Z

example/jwks-verify.cpp

+    auto verifier = jwt::verify()
+      .allow_algorithm(jwt::algorithm::rs256(jwt::helper::convert_base64_der_to_pem(x5c), "", "", ""))
+      .with_issuer(issuer)
+      .leeway(10000000000UL); // value in seconds, add some to compensate timeout


Perhaps, a simple token with no expiration is possible?

example/jwks-verify.cpp

include/jwt-cpp/jwt.h

prince-chrismc · 2020-12-07T19:29:39Z

include/jwt-cpp/jwt.h

+
+	public:
+		JWT_CLAIM_EXPLICIT jwks(const typename json_traits::string_type& jwks_token) {
+			auto parse_claims = [](const typename json_traits::string_type& str) {


this block seems to be copy pasted from the original

📓 for myself to look into a small refactor!

yes, and it costs an extra move, good point to refactor.

include/jwt-cpp/jwt.h

prince-chrismc · 2020-12-07T19:45:35Z

include/jwt-cpp/jwt.h

+	};
+
+	template<typename json_traits>
+	class jwks_keys {


I wonder if this "parser" could be a static method that returns a vector, this was the implementation is not limited on KIDs being used in all the keys

Thoughts?

to_vector() is added

Co-authored-by: Chris Mc <[email protected]>

prince-chrismc

The changes look really good! 👑

There's a few more details about x5c handling I would appreciate your thoughts on.

Lastly, I need to get my note book and review them, I may come back with a suggestion to make the jwks more generic... there's a lot of variety with the content and I want to make sure. Hopefully you can understand!

Keep up the awesome work 💪

prince-chrismc · 2020-12-10T23:17:11Z

include/jwt-cpp/jwt.h


 		/**
 		 * Get x5c claim
 		 * \return x5c as string
 		 * \throw std::runtime_error If claim was not present
 		 * \throw std::bad_cast Claim was present but not a string (Should not happen in a valid token)
 		 */
-		typename json_traits::string_type get_x5c() const { return json_traits::as_string(get_jwks_claim("x5c").as_array().front()); } //do not use serialize() instead
+		typename json_traits::string_type get_x5c() const { return json_traits::as_string(get_jwk_claim("x5c").as_array().front()); } //do not use serialize() instead


I think the consumer will/may need the array in larger systems as per the rfc 4.7

The PKIX certificate containing the key value MUST be the first
certificate. This MAY be followed by additional certificates, with
each subsequent certificate being the one used to certify the
previous one.

What do you think about having two functions?

typename json_traits::array_type get_x5c() const; typename json_traits::string_type get_x5c_key_value() const;

Secondly, front might be a little dangerous! Calling front on an empty container is undefined

please throw if empty 💥

prince-chrismc · 2020-12-10T23:22:20Z

include/jwt-cpp/jwt.h

+				typename json_traits::string_type error_msg = "jwk with key id \"";
 				error_msg += key_id;
 				error_msg += "\" not found";
 				throw std::runtime_error(error_msg.c_str());


the caller knows the kid they passed, no need to repeat it

Suggested change

typename json_traits::string_type error_msg = "jwk with key id \"";

error_msg += key_id;

error_msg += "\" not found";

throw std::runtime_error(error_msg.c_str());

throw std::runtime_error("jwk with matching key id not found");

prince-chrismc · 2020-12-10T23:24:53Z

include/jwt-cpp/jwt.h


 	private:

-		std::unordered_map<typename json_traits::string_type, jwks_t> jwks_keys_claims;
+		std::unordered_map<typename json_traits::string_type, jwk_t> jwks_claims;


Only because i've implemented this and used a few different auth servers... I am really unsure about this being a map...

What if there are two keys neither have KIDs? One would be lost, most likely the new key that is starting to issue tokens.

prince-chrismc · 2020-12-14T19:47:48Z

include/jwt-cpp/jwt.h

+				for (const auto& k : json_traits::as_object(val)) {
+					size_t id = 0;
+					for (const typename json_traits::value_type& item : json_traits::as_array(k.second)) {
+						jwk_t jwk_entry(item);
+						if (jwk_entry.has_key_id())
+							res.emplace(jwk_entry.get_key_id(), std::move(jwk_entry));
+						else
+							res.emplace(std::to_string(id), std::move(jwk_entry));
+						++id;
+					}
+				}


Yep, so I would really like if we could leave the KID outside of the JWKS logic because it's important to support workflows that do not required them.

I will come back with some changes against your branch, I think the biggest limitation is the parsing or "map of claims"

I also see we don't check "keys" explicitly is the JWKS which would open use to a vulnerability

prince-chrismc · 2020-12-15T02:11:07Z

@dtiukalov please consider dtiukalov#1

rasmuspeders1 · 2021-05-25T06:31:54Z

This looks great! Basically this is exactly what I was looking for. A single nice c++ library to handle jwt auth including jwk handling.
Looking forward to trying it out when it is ready.

prince-chrismc · 2021-05-25T12:27:00Z

Looking forward to trying it out when it is ready.

Sadly it seems OP has stopped working on this, it would be greatly appreciated if you could pick up and work on this!

Lots of work has been done just needs a little push to get it over

rasmuspeders1 · 2021-05-25T12:38:52Z

Looking forward to trying it out when it is ready.

Sadly it seems OP has stopped working on this, it would be greatly appreciated if you could pick up and work on this!

Lots of work has been done just needs a little push to get it over

That is too bad.
I am not sure I will have the time in the near future nor possess the necessary skill. I will keep it in mind if I find some time.

ghost · 2021-06-28T16:48:15Z

@prince-chrismc what's left in this PR? maybe i can take a shot at this

prince-chrismc · 2021-06-28T20:21:47Z

I attempted to help the original author and made a PR against his branch.... https://github.com/prince-chrismc/jwt-cpp/tree/add-jwks is the latest code

Sadly it's been a while and I do not recall the details...
Scrolling over the conversation, I had some concerns the implementation was limiting some JOSE workflows... I see some test code but more is always better

If you have time to push this feature I'd love to help!

Thalhammer · 2021-06-28T20:47:12Z

It will probably also need a pretty significant update to the current version.

ghost · 2021-06-29T02:41:55Z

Does it make sense to start a new PR with OP's changes + @prince-chrismc 's suggestions to it, targetting the latest version? I think looking at the latest diff might help us see what's left

Update jwt.h

0366d4c

add jwks implementation to simple access to x5c to verify jwt

prince-chrismc self-assigned this Dec 2, 2020

dtiukalov added 14 commits December 7, 2020 13:33

add multi keys in jwks support

099c249

bug fix

1e16c10

add missed '}'

a6ccd9a

add JwksTest

ffa40c9

Create JwksTest.cpp

901a6a7

replace AnsiString by std::string

df53292

fix typo

7b370ce

wrap fake kid with ASSERT_THROW

e54dc12

fix typo

b20a114

fix typo

cda5166

Update JwksTest.cpp

ed5e307

Update JwksTest.cpp

ca30c1c

Create jwks-verify.cpp

11a5717

Update CMakeLists.txt

3db45cb

prince-chrismc reviewed Dec 7, 2020

View reviewed changes

dtiukalov and others added 10 commits December 9, 2020 10:59

Update example/jwks-verify.cpp

9939c8c

Co-authored-by: Chris Mc <[email protected]>

Update include/jwt-cpp/jwt.h

c1b86f4

Co-authored-by: Chris Mc <[email protected]>

jwks -> jwk; jwks_keys -> jwks

f5dad55

jwks -> jwk; jwks_keys -> jwks

d72d592

jwks -> jwk; jwks_keys -> jwks

609f6f6

Merge branch 'master' into patch-1

e06fc33

bug fix

d50c25e

Update JwksTest.cpp

c5fb84f

add to_vector for jwks; add support key_id'less

ca53842

std::string(id) -> std::to_string(id)

ac48132

prince-chrismc reviewed Dec 10, 2020

View reviewed changes

prince-chrismc reviewed Dec 14, 2020

View reviewed changes

prince-chrismc mentioned this pull request Dec 15, 2020

refactor parsing of claims + jwks as array dtiukalov/jwt-cpp#1

Closed

Thalhammer changed the title ~~Update jwt.h~~ Add jwks parsing Feb 2, 2021

prince-chrismc removed their assignment Mar 24, 2021

ghost mentioned this pull request Jun 29, 2021

Add JWKS parsing #146

Merged

Thalhammer changed the title ~~Add jwks parsing~~ Add jwks parsing (abandoned) Jun 29, 2021

Thalhammer closed this in #146 Jul 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jwks parsing (abandoned) #112

Add jwks parsing (abandoned) #112

dtiukalov commented Dec 2, 2020 •

edited by prince-chrismc

Loading

prince-chrismc commented Dec 2, 2020

prince-chrismc Dec 7, 2020

prince-chrismc Dec 7, 2020

dtiukalov Dec 9, 2020

prince-chrismc Dec 7, 2020

dtiukalov Dec 9, 2020

prince-chrismc left a comment

prince-chrismc Dec 10, 2020

prince-chrismc Dec 10, 2020

prince-chrismc Dec 10, 2020

prince-chrismc Dec 14, 2020

prince-chrismc commented Dec 15, 2020

rasmuspeders1 commented May 25, 2021

prince-chrismc commented May 25, 2021

rasmuspeders1 commented May 25, 2021

ghost commented Jun 28, 2021

prince-chrismc commented Jun 28, 2021

Thalhammer commented Jun 28, 2021

ghost commented Jun 29, 2021

Add jwks parsing (abandoned) #112

Add jwks parsing (abandoned) #112

Conversation

dtiukalov commented Dec 2, 2020 • edited by prince-chrismc Loading

prince-chrismc commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prince-chrismc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prince-chrismc commented Dec 15, 2020

rasmuspeders1 commented May 25, 2021

prince-chrismc commented May 25, 2021

rasmuspeders1 commented May 25, 2021

ghost commented Jun 28, 2021

prince-chrismc commented Jun 28, 2021

Thalhammer commented Jun 28, 2021

ghost commented Jun 29, 2021

dtiukalov commented Dec 2, 2020 •

edited by prince-chrismc

Loading